feat(opp-solana): drive resumable inbound dispatch; delete the depot-side terminal budget - #552
feat(opp-solana): drive resumable inbound dispatch; delete the depot-side terminal budget#552valthon wants to merge 3 commits into
Conversation
heifner
left a comment
There was a problem hiding this comment.
Review of the resumable-dispatch change: 10 findings, ranked by severity. Nine are anchored inline on the diff; one sits in a file this PR does not touch and is included here.
Retry idempotence / fee burn — plugins/batch_operator_plugin/src/outpost_opp_job.cpp:120 (file not in this diff)
Nothing records that chunks were already staged, and _last_outbound_epoch only advances on a clean return (outpost_opp_job.cpp:120-134). Every throw path in the new, much longer delivery (deadline, program abort, decode failure, stale cursor read) leaves it unset, so the cron re-enters deliver_outbound_envelope every 15 s, which unconditionally re-sends all data chunks from offset 0 (outpost_solana_client.cpp:908-923) before reaching the terminal loop. For a wedged epoch that is an unbounded fee burn and RPC load — 4 tx/tick for the dev-026 envelope, 99 tx/tick at the 64 KiB cap — with no backoff, and the re-upload itself eats into the same 15 s budget, making the deadline-throw wedge self-reinforcing. Suggest probing the chunk_buffer PDA (or a local memo) before re-uploading.
31046a1 to
3b0ad72
Compare
A containerised build leaves two directories in the work tree that are
not source and must never be committed:
* `.pnpm-store/` -- the OPP bundle generator's pnpm store, materialised
inside the repo because the container has no writable store elsewhere.
* `.container-home/` -- a throwaway HOME the build container needs
because it runs as a non-root uid with no home directory of its own.
Both showed up as untracked noise in every `git status` taken during a
container build, which is exactly the failure mode `.gitignore` exists
to prevent: real untracked work hidden behind build residue.
Unrelated to the rest of this branch; kept separate so it can be taken
or dropped on its own.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Change-Id: I92cae4240f6b6e39c8c1b13fd27c930c668ba9b5
A Solana transaction cannot carry the effect accounts for every
attestation in an OPP envelope: the packet limit is 1232 bytes and each
dynamic account costs ~33, so a large envelope -- many SPL swap remits
across distinct reserves -- needs more accounts than one transaction can
declare. The fused design made the terminal `epoch_in` both record the
delivery and dispatch every effect, so every way dispatch could fail
(account budget, decode failure, missing account) became a way consensus
could wedge.
Split the two jobs, mirroring the program's `epoch_in` /
`dispatch_attestations` instruction split, and contain the entire relay
side in the solana client -- `outpost_solana_client.{hpp,cpp}` and its
tests -- with ZERO plugin-interface changes (architecture review,
2026-08-11: no new SPI virtuals, no new cron jobs, no shell-header
additions; see docs/superpowers/specs/):
* `deliver_outbound_envelope` stages 668-byte chunks (the program's
MAX_CHUNK_BYTES; 672 no longer fits with the tx overhead) and sends
a zero-account terminal call, so delivery is fixed-size and cannot
fail on a dispatch concern. It then best-effort drains inline --
log-and-drop -- so the operator whose delivery tips consensus
settles immediately without a settlement failure ever reading as a
delivery failure (which would re-upload every chunk each tick).
* `read_inbound_envelope` drains before reading. `run_inbound`
already calls it every tick for exactly as long as the epoch's
outbound envelope is missing -- and that envelope only exists once
the cursor drains -- so the read path is the standing recovery
driver: any elected operator's tick can unstick a wedged cursor.
The envelope bytes come from a delivered-envelope memo, repopulated
after a restart by the next outbound tick's redelivery.
* `dispatch_attestations` is sent through the program client's public
generic API (get_idl / resolve_accounts / execute_tx_and_confirm);
the crank loop (`drive_dispatch_rounds`) packs greedily from the
on-chain cursor -- the cursor is the ONLY position input, so a
re-drive after a deadline, restart, or another operator's partial
drain resumes where the program will actually settle. Cursor reads
pin `processed` commitment to match the write. Round-budget
exhaustion elogs and returns; the next tick resumes.
* `extract_inbound_effects` is the single authoritative envelope
decode, indexed by the flat dispatch-order position the cursor
counts. A locally undecodable envelope THROWS rather than reading
as empty. Boot asserts the IDL carries `dispatch_attestations` so a
stale program deployment fails at startup, not at the first drain.
Unit tests drive the crank state machine through its callback seam
(multi-round drain, nonzero-cursor resume, stall/consensus exits,
zero-attestation close, exhaustion) and measure the packet-limit
budgets against the IDL fixture. Verified end-to-end by
flow-swap-from-wire: epochs advance with all four OPP directions
growing and zero fatal signatures.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Change-Id: I6e21005ef85f1f2eeac1423bbf81ac93a6d13f30
Now that Solana dispatch is resumable (previous commit), the depot no
longer has to model Solana's transaction capacity. The outpost settles
`[cursor, cursor + dispatch_limit)` per call and the relay loops until
the cursor drains, so an envelope needing more effect accounts than one
Solana transaction can carry is settled across several calls -- it no
longer has to be refused when it is built.
Deleted:
* `estimate_svm_dynamic_accounts` -- a per-attestation-type account
estimator living in a WIRE consensus contract.
* The `svm_terminal_budget_fits` gate in `buildenv`'s packing loop,
which truncated an outbound envelope to fit a Solana packet.
* The `queueout` admission check. This was the sharper half: it did
not cap packing, it REFUSED TO QUEUE any Solana-bound attestation
the estimator did not recognise, making the SEC-94 manifest an
allow-list over what could ever be sent to Solana. Adding an
attestation type meant editing a depot contract's estimator or the
depot rejected it outright.
* `solana_terminal_budget.hpp` and
`tests/fixtures/sec-94-solana-terminal-budget.json`.
Two contract tests asserted the deleted behaviour and are deleted
rather than re-baselined, because their premise is a budget that no
longer exists:
`buildenv_svm_packs_until_terminal_budget_then_leaves_remainder`
(expected 2 of 3 SWAP_REMITs emitted with a remainder left behind; it
now emits all three, which is the point) and
`queueout_svm_rejects_uncovered_attestation_type`.
`UNCOVERED_TEST_ATTESTATION_TYPE` and `emitted_attestation_count` lost
their last callers and go with them.
The packing loop keeps its `MAX_ENVELOPE_BYTES` bound and gains a
comment stating why there is deliberately no destination-chain bound,
so the next reader does not reintroduce one.
`queueout` keeps a chain_code registration assert. The
`chains_tbl.get()` beside the estimator existed only to learn
`chain.kind`, so it fell out with the gate by accident rather than by
decision. The assert is not an attack gate -- the authorization check
above it is -- it is an ops gate: a governance msig can call `queueout`
directly, and a typo'd code used to abort here. Without it the row is
created READY and unreachable forever -- permanent sysio-pool RAM,
re-walked by the Phase-1 bystatus scan on every `buildenv`. Two bare
`chains_tbl.get()` calls (in `buildenv` and the inbound-envelope path)
gain message overloads.
The ABI is unchanged, so no `SysioContractTypes` regeneration is
needed. The checked-in `sysio.msgch.wasm` is rebuilt from this source;
it grows ~19% only because it comes from a different CDT build (the
same toolchain delta inflates untouched contracts similarly), and
`contracts_unit_test` is green at 617/617 (including the UIC prevalidation tests merged from master).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Change-Id: Ic3af326b86e8e446609214ea7936bd0c325dfc89
3b0ad72 to
b349fe1
Compare
|
Rebased, addressed all comments, and new flow tests 13/13 ✅ |
heifner
left a comment
There was a problem hiding this comment.
Re-review at b349fe1414 (rebased onto dade24c613).
This is not a patch-up of the version I reviewed — it is a different architecture. epoch_in is now delivery-only (the dispatch_limit argument is gone entirely) and settlement moved to a new permissionless dispatch_attestations instruction driven from the on-chain cursor. That restructuring is what closes the prior findings, and it closes them at the root rather than at the symptom. All ten are addressed.
Prior findings
| # | Finding | Status |
|---|---|---|
| 1 | Retry re-uploads all chunks on every drain failure | Resolved by design. The terminal epoch_in is fixed-size with no manifest, and the inline drain is try/caught (outpost_solana_client.cpp:1095-1107) so settlement can no longer fail the delivery. Re-upload now requires an actual chunk / terminal-call failure — the same surface as the base. |
| 2 | settled never seeded from the cursor → multi-round wedge |
Fixed. drive_dispatch_rounds reads the cursor before packing (:597, :612). Pinned by drive_dispatch_resumes_from_nonzero_cursor. |
| 3 | Zero-attestation / decode-failure path never records the delivery | Fixed, and better than asked. Delivery records regardless of decode; decode failure now throws inside drain_dispatch where it belongs (:993-1001); an empty envelope gets one clamped close crank, since the program's completion block is the only place next_epoch_index advances. The matching program-side regression test (dispatch_window(0, 0, 5) == (0, 0)) is the right place for that invariant to live. |
| 4 | Cursor read at confirmed against a write confirmed at processed |
Fixed. The read is pinned to commitment_t::processed (:748). |
| 5 | Silent round exhaustion; budget not sized against the depot's worst case | Fixed. Exhaustion elogs (:689-692), the budget went 64 → 128 with a stated sizing rationale, and resumption is now real via drain-then-read. |
| 6 | MAX_TERMINAL_DYNAMIC_ACCOUNTS = 16 left unmeasured after the SEC-94 fixture was deleted |
Fixed. dispatch_attestations_full_manifest_fits_packet_limit measures the real shape — IDL statics, fee payer, the heap-frame pre-instruction, and 16 distinct writable extras. |
| 7 | Conflicts with the standing outpost-remit-is-immediate rule |
Addressed; one item outstanding — see below. |
| 8 | queueout lost its only chain_code registration check |
Fixed. The check is back with its original message, and buildenv / dispatch_attestation now carry explicit assert messages instead of the bare key not found. |
| 9 | The dispatch state machine shipped untested; five dead extractor views | Fixed. The loop is factored over its RPC seams with 12 dedicated tests, and the dead views are deleted. |
| 10 | Lockstep break against the manifest-pinned program | Fixed. wire-solana#419 now carries MAX_CHUNK_BYTES = 668 and dispatch_attestations. |
The e2e evidence checks out independently: run 31550041513 succeeded with BRANCH_WIRE_SYSIO and BRANCH_WIRE_SOLANA both set to feat/resumable-opp-dispatch and was dispatched after the head commit — the correct paired combination for a coupled change.
New findings
Four on the reworked design, anchored inline. Two of them (the memoize placement and the eager manifest build) are worth fixing before merge; the other two are cleanups.
Please remove docs/superpowers/plans/2026-08-11-contained-resumable-dispatch-relay.md
That file is session scaffolding rather than repository documentation: a checkbox task list addressed "For agentic workers", pinning an absolute local path (/home/valthon/wire-platform/wire-sysio) and a commit-by-commit workflow that is already history. It will read as normative to the next person who greps docs/ and finds it.
docs/opp-two-phase-dispatch-design.md is the genuine design doc and should absolutely stay — it is the artifact that makes this change reviewable, and its outpost-remit-is-immediate section is doing real work. docs/superpowers/specs/2026-08-11-contained-resumable-dispatch-relay-design.md is a placement redirect for the plan; if the plan goes, it is worth asking whether the redirect still has a reader.
Outstanding landing item
wire-platform-manifest/.claude/rules/outpost-remit-is-immediate.md is still unedited, and continues to state: "There is no concept of 'pending remit,' 'queued release,' … the dispatch loop IS the release."
The design doc handles the tension honestly — there is an explicit "Relationship to outpost-remit-is-immediate" section that concedes the design fails the rule's letter and argues its intent, with the rule update listed as landing step 4 — and wire-solana#419 has already updated opp-outpost-technical-spec.md (+106/−57). Only the manifest rule is left. Worth landing in the same window, since every diagnosis playbook built on that rule will otherwise misread production.
Verdict: the architecture change is the right call, and the tests now pin the exact region where both confirmed bugs lived. Nothing here blocks the coordinated merge with wire-solana#419.
|
|
||
| // Memoize BEFORE the sends: even a partially-staged delivery leaves this | ||
| // relay able to crank once consensus tips via other operators. | ||
| _delivered_envelope = std::make_pair(epoch_index, envelope_bytes); |
There was a problem hiding this comment.
The memo is set before the chunks are staged, and a partially-staged buffer cannot be cranked — the comment asserts the opposite.
The comment reads "Memoize BEFORE the sends: even a partially-staged delivery leaves this relay able to crank once consensus tips via other operators." The program refuses exactly that case: read_staged_envelope (wire-solana instructions/opp/mod.rs:381-384) requires parsed.received_chunks == parsed.total_chunks, else TerminalChunkBeforeDataComplete; a buffer that was never created fails the owner check one step earlier.
So when the chunk loop throws partway (RPC hiccup, deadline, validator hiccup), _delivered_envelope is already set and the delivery propagates out to outpost_opp_job::run_outbound's catch with _last_outbound_epoch unset. Every read_inbound_envelope tick until the next successful outbound then takes the drain-then-read path at :1130, builds the manifest, and — once consensus tips via other operators — sends dispatch_attestations against our own incomplete buffer. skip_preflight is false (libfc solana_client.cpp:1726), so it is rejected at simulation rather than landing: no fee, but a guaranteed-to-fail RPC round trip plus a wlog on every inbound tick in that window.
Bounded — the next run_outbound tick re-uploads from offset zero and repairs it — but the window is real and the comment points the next reader the wrong way.
Suggest moving the assignment below the chunk loop (before or after the terminal epoch_in). The goal the comment actually wants — crank when our terminal call or drain failed but the buffer is complete — is served either way, and the partial case it claims to cover is not reachable by design.
| // chain-side reason and the read proceeds -- an undrained epoch simply | ||
| // reads back empty via the stale-epoch check below, and the next tick | ||
| // resumes from the on-chain cursor. Consensus-not-reached is a cheap | ||
| // internal no-op (one progress read) inside the drive loop. |
There was a problem hiding this comment.
The cost claim here is wrong: the drain builds the full effect manifest before it ever checks consensus or the cursor.
"Consensus-not-reached is a cheap internal no-op (one progress read) inside the drive loop" is true of drive_dispatch_rounds — but drain_dispatch does all of its manifest work first. extract_inbound_effects walks the envelope, then accounts_for_effect runs per effect, and every reserve-backed shape reaches reserve_info_for_codes, which costs two get_account_info calls (the Reserve PDA, then config_pda; the per-(token, reserve) cache does not span calls, and the config read sits inside the cached function rather than beside it).
For every operator except the one whose delivery tips consensus, that entire manifest is built and discarded — on every inbound tick, for as long as the epoch's outbound envelope is missing, which is precisely the window this call is designed to run in.
Scoped honestly: a steady-state envelope carrying only OPERATORS / BATCH_OPERATOR_GROUPS produces no reserve-backed effects and costs nothing extra. This bites on swap and reserve epochs — the busy ones, where the same envelope also has the most effects to walk.
Reading progress once at the top of drain_dispatch and returning early on !consensus_reached || dispatched_count >= total_attestations fixes both the cost and the comment, and subsumes the redundant pre-loop read flagged at :597.
| // correctly instead of re-claiming the window from zero. | ||
| uint32_t settled = read_progress().dispatched_count; | ||
| for (uint32_t round = 0; round < MAX_DISPATCH_ROUNDS; ++round) { | ||
| if (total_attestations > 0 && settled >= total_attestations) return last_sig; |
There was a problem hiding this comment.
The pre-loop cursor read and this guard are both dead — one get_account_info per drain with no behavioural contribution.
settled from :597 is consumed only by this line. Three lines down, :605 re-reads the same account and :612 overwrites settled before any other use, so the value from :597 never reaches the packing loop.
And this guard cannot fire for round ≥ 1 either: the bottom of the loop already returns on settled >= total_attestations (:680), so re-entry guarantees settled < total_attestations.
Initialising settled = 0 and dropping both lines is behaviour-identical. Walked against all twelve drive_dispatch_* expectations:
already_drained_cursor_sends_nothing— falls through to:605, consensus is reached,:612setssettled = 3,:631returns without sending. Same outcome.consensus_not_reached_sends_nothing— reaches the:606check one read earlier and returns. Same outcome.resumes_from_nonzero_cursor— unaffected;:612is what seeds the resume, which is the fix for the original finding.
The only observable delta is that an already-drained epoch whose consensus flag is false now logs "consensus not yet reached" instead of returning silently, which is if anything the more accurate line. Worth doing because this drain now runs on every inbound tick, so the saved read is per-operator per-tick rather than once per envelope.
| // on-chain cursor on a later drain. The failure is logged, never swallowed | ||
| // -- the chain-side reason is the diagnostic. | ||
| try { | ||
| drain_dispatch(epoch_index, envelope_bytes, deadline); |
There was a problem hiding this comment.
This hands drain_dispatch the original deadline duration rather than the remaining budget — the read path gets it right, this one does not.
read_inbound_envelope computes deadline_abs - fc::time_point::now() (:1132-1133); here the full deadline is passed after the chunk uploads have already consumed part of it, so drain_dispatch installs a fresh budget at :862.
Wall-clock is still capped, so this is not an overrun: fc::task::deadline_scope keeps the earliest of the previous and new deadlines (libfc fc/task/deadline.hpp:31-36), and http_client consults fc::task::current_deadline() (http_client.cpp:147), so RPCs still throw on the caller's real deadline.
The cost is diagnostic. The loop's own throw_if_past_deadline(deadline_abs, OP_DISPATCH_ATTESTATIONS) at :1021 can never fire first, so the OP_DISPATCH_ATTESTATIONS label — added by this PR specifically to identify this phase — is unreachable, and an operator whose drain runs long sees a generic transport timeout instead. Matching the read path's deadline_abs - fc::time_point::now() restores it.
| @@ -0,0 +1,648 @@ | |||
| # Contained Resumable-Dispatch Relay Implementation Plan | |||
There was a problem hiding this comment.
Please drop this file from the PR.
It is session scaffolding rather than repository documentation — a checkbox task list addressed "For agentic workers", pinning an absolute local path (/home/valthon/wire-platform/wire-sysio), a build directory, and a commit-by-commit workflow that is already history. Its constraints are also written against a 114bfbd486 baseline that this branch has since been rebased off, so anyone who finds it under docs/ will be reading instructions that no longer describe the tree.
docs/opp-two-phase-dispatch-design.md is the genuine design doc and should stay — it is what makes this change reviewable. If this plan goes, docs/superpowers/specs/2026-08-11-contained-resumable-dispatch-relay-design.md is worth a second look too, since it exists to redirect this plan's placement.
Important
Must merge together with https://github.com/Wire-Network/wire-solana/pull/419.
SOLANA_MAX_CHUNK_BYTEShere andMAX_CHUNK_BYTESthere are one coupled protocolconstant:
epoch_inrequires every non-final chunk to be exactly that size, somerging either side alone breaks multi-chunk envelopes with
ChunkSizeMismatch.The relay also cannot send
dispatch_limituntil the program accepts it.Relay side of resumable OPP dispatch. Pairs with Wire-Network/wire-solana#419 —
they must land together.
What changes
buildenv's packing gateand
queueout's admission check were capping what could ever be sent to Solana, basedon a depot-side estimate of the outpost's account budget. Chunking plus a per-call
dispatch limit replaces it, so the depot no longer needs to model the outpost's
transaction shape.
epoch_incarriesdispatch_limit, and the terminal call loops until the on-chaindispatched_countcursor drains. Each round greedily packs attestations while theiraccount union still fits, always taking at least one so an oversized effect makes
progress rather than wedging the epoch. Three exits: cursor drained, consensus not yet
reached, or the cursor failed to advance (another operator is draining it).
extract_inbound_effects()is now the authoritative indexed walk of the envelope;the five per-effect extractors became dedup views over it, so per-attestation account
manifests stay aligned with attestation indices.
Packet budget
SOLANA_MAX_CHUNK_BYTESdrops 672 -> 668.dispatch_limitadds 4 bytes to everyepoch_incall including data chunks, where it is ignored — that pushed a full chunk from1230 to 1234 bytes against Solana's 1232-byte limit, so
solana_types.cpp::serializethrew host-side before any RPC. Coupled by protocol to
MAX_CHUNK_BYTESin wire-solana:non-final chunks must be exactly this size so offsets line up at
chunk_index * MAX_CHUNK_BYTES. Overhead is 562 bytes, ceiling 670; 668 restores the2-byte margin that existed before
dispatch_limit.Adds
epoch_in_full_data_chunk_fits_packet_limit, which serializes a full data chunk andasserts it fits. This replaces the SEC-94 fixture parity test and covers changes to the
instruction's argument tuple, not just its account list — the old fixture's contract
was account-list-only, so it would not have caught this. Verified to fail at 672 and pass
at 668.
Verification
test_outpost_solana_client_plugin— No errors detected(built against wire-solana
fc061a46+ wire-sysio00e456ee; every other repo'srevision identical to the prior run, so the result is attributable to this change)